Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Swift: upgrade to 5.9 #14261

Merged
merged 8 commits into from
Nov 8, 2023
Merged

Swift: upgrade to 5.9 #14261

merged 8 commits into from
Nov 8, 2023

Conversation

AlexDenisov
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the Swift label Sep 19, 2023
@AlexDenisov AlexDenisov force-pushed the alexdenisov/upgrade-to-swift-5.9 branch from fec24b9 to 1bd3b91 Compare September 19, 2023 14:45
@AlexDenisov AlexDenisov force-pushed the alexdenisov/upgrade-to-swift-5.9 branch 2 times, most recently from 645a3d6 to 8b55cea Compare October 11, 2023 09:14
@AlexDenisov AlexDenisov mentioned this pull request Oct 11, 2023
Merged
@AlexDenisov AlexDenisov force-pushed the alexdenisov/upgrade-to-swift-5.9 branch 2 times, most recently from e63cd58 to f3f244c Compare October 17, 2023 12:20
@AlexDenisov AlexDenisov mentioned this pull request Oct 18, 2023
Merged
@AlexDenisov AlexDenisov force-pushed the alexdenisov/upgrade-to-swift-5.9 branch from 1ab4a21 to a96ac14 Compare October 31, 2023 10:24
@AlexDenisov AlexDenisov force-pushed the alexdenisov/upgrade-to-swift-5.9 branch from 992869e to 00235df Compare November 7, 2023 11:44
@AlexDenisov AlexDenisov force-pushed the alexdenisov/upgrade-to-swift-5.9 branch from 00235df to 2b7ce23 Compare November 7, 2023 12:40
@AlexDenisov AlexDenisov marked this pull request as ready for review November 7, 2023 14:22
@AlexDenisov AlexDenisov requested review from a team as code owners November 7, 2023 14:22
@AlexDenisov AlexDenisov mentioned this pull request Nov 7, 2023
Closed
redsun82
redsun82 reviewed Nov 7, 2023
View reviewed changes
swift/schema.py Outdated

class SingleValueStmtExpr(Expr):
"""
An expression that may wrap a statement which produces a single value.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"may" makes it sound like the statement would be optional. Maybe

Suggested change
An expression that may wrap a statement which produces a single value.
An expression that wraps a statement which produces a single value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what the docs from compiler sources said 🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, then we should probably double-check whether Stmt should not be optional (I would be surprised if it could though). If not, then I would drop the may regardless of the internal compiler docs (which by the way we shouldn't really copy 😉).

geoffw0
geoffw0 reviewed Nov 7, 2023
View reviewed changes
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The library changes and upgrade / downgrade scripts LGTM.

swift/ql/test/library-tests/dataflow/flowsources/CONSISTENCY/CfgConsistency.expected Outdated Show resolved Hide resolved
@geoffw0
Copy link
Contributor

geoffw0 commented Nov 7, 2023

I'm happy, once @redsun82 and the checks and the DCA job are all happy.

@AlexDenisov AlexDenisov added the no-change-note-required This PR does not need a change note label Nov 8, 2023
@AlexDenisov
Copy link
Contributor Author

@geoffw0 @redsun82 anything else you'd like to see?

@geoffw0
Copy link
Contributor

geoffw0 commented Nov 8, 2023

Well I don't understand what's going on with the change note, but I remain happy with this PR. @redsun82 can approve if his concerns have been addressed.

the QLDoc failure can be ignored as it will be fixed after #14715

I think when one of these two PRs is merged, the other will need updating.

@AlexDenisov
Copy link
Contributor Author

Well I don't understand what's going on with the change note, but I remain happy with this PR

AFAICT, currently we only have space for QL-related change notes. I'd like to include a note saying that we now support Swift 5.9, but previously I had to talk to a release manager to make this happen.

I think when one of these two PRs is merged, the other will need updating.

For sure, I hope this one gets in first though 😅

@AlexDenisov AlexDenisov requested a review from redsun82 November 8, 2023 09:31
@redsun82
Copy link
Contributor

redsun82 commented Nov 8, 2023

unless we need to add that we now extract one more AST node

More or less this. From the library perspective, there is a new class, so it's probably worth mentioning that. * added `SingleValueStmtExpr`. would probably be enough, I think?

@AlexDenisov
Copy link
Contributor Author

unless we need to add that we now extract one more AST node

More or less this. From the library perspective, there is a new class, so it's probably worth mentioning that. * added `SingleValueStmtExpr`. would probably be enough, I think?

I'll do in the followup PR then, I don't want to restart the whole CI process for this PR. (unless there is some other changes need to happen here).

@geoffw0
Copy link
Contributor

geoffw0 commented Nov 8, 2023

AFAICT, currently we only have space for QL-related change notes. I'd like to include a note saying that we now support Swift 5.9, but previously I had to talk to a release manager to make this happen.

I feel that the library change notes are an appropriate place to describe extractor changes (i.e. swift\ql\lib\change-notes), if we do not have separate extractor change notes.

@AlexDenisov
Copy link
Contributor Author

@redsun82 @geoffw0 I added change note.

@AlexDenisov AlexDenisov removed the no-change-note-required This PR does not need a change note label Nov 8, 2023
geoffw0
geoffw0 previously approved these changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready to merge. Please check that @redsun82 agrees before merging.

@MathiasVP
Merge pull request #14717 from github/alexdenisov/fix-cfg-for-single-…
d8e75e6 
…value-stmt-expr

Swlft: fix CFG for SingleValueStmtExpr
@redsun82 redsun82 self-requested a review November 8, 2023 15:15
redsun82
redsun82 approved these changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@redsun82 redsun82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the terrific work on this!

@redsun82 redsun82 merged commit e68826b into main Nov 8, 2023
18 of 19 checks passed
@redsun82 redsun82 deleted the alexdenisov/upgrade-to-swift-5.9 branch November 8, 2023 15:15
@geoffw0 geoffw0 mentioned this pull request Nov 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@redsun82 redsun82 redsun82 approved these changes

@geoffw0 geoffw0 geoffw0 left review comments

Assignees
No one assigned
Labels
documentation Swift
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlexDenisov @geoffw0 @redsun82 @MathiasVP